WIP: Added ase Nudged Elastic Band in the NewtonNet recipe #2176

kumaranu · 2024-05-24T02:50:24Z

Summary of Changes

>>
Target here is to add ase's NEB as the double ended transition state search method for NewtonNet recipe.

I need to make sure that geodesic code is added as a preprocessor for the double ended transition state search.
NEB will take the output of Geodesic
Sella will them run from the top point of NEB.
Tests will need to be added for all the files keeping the coverage of more than 99% before taking it for review.
Currently, there is a lot of unstructured code in the ts.py and in the test. This will need to be cleaned up to match with the rest of the recipe's in QuAcc.
The summary dictionaries will need to be written so that the data can be passed back to the database.
Also, the requirements will be added in the NewtonNet's recipe, however, it is a general problem where ideally the calculator can be changed. If a decision is made later on to move it out of NewtonNet then things will (hopefully) be still usable a general path optimization process and can be added along with the static, relax, freq, and thermo jobs. But, right now, NewtonNet seems like an OK place for this to be put in.

<<

Checklist

I have read the "Guidelines" section of the contributing guide. Don't lie! 😉
My PR is on a custom branch and is not named main.
I have added relevant, comprehensive unit tests.

Notes

Your PR will likely not be merged without proper and thorough tests.
If you are an external contributor, you will see a comment from @buildbot-princeton. This is solely for the maintainers.
When your code is ready for review, ping one of the active maintainers.

buildbot-princeton · 2024-05-24T02:50:27Z

Can one of the admins verify this patch?

codecov · 2024-05-24T03:00:24Z

Codecov Report

Attention: Patch coverage is 96.98795% with 5 lines in your changes missing coverage. Please review.

Project coverage is 97.38%. Comparing base (7fef05b) to head (c67c622).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/quacc/runners/ase.py	92.10%	3 Missing ⚠️
src/quacc/schemas/ase.py	93.93%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2176      +/-   ##
==========================================
- Coverage   97.41%   97.38%   -0.03%     
==========================================
  Files          85       86       +1     
  Lines        3518     3673     +155     
==========================================
+ Hits         3427     3577     +150     
- Misses         91       96       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Andrew-S-Rosen

Hi @kumaranu: I know this is still a WIP but I wanted to provide some initial comments. The biggest thing to note is that in quacc you cannot write things out to the current working directory or any relative paths. Quacc does not cd into the working directory and so there is no guarantee that the current working directory or any relative path is going to be where the runtime/results directory is.

If you are looking to write out files before the calculation completes, then you need write it out based on the tmpdir that is made when quacc.runners.pre.calc_setup is called. For instance, in phonopy, you can see that certain files are written out (e.g. phonopy.yaml, phonopy_auto_band_structure.yaml) based on the tmpdir of the runtime directory:

quacc/src/quacc/runners/phonons.py

Lines 55 to 68 in 04af31e

    
           # Perform staging operations 
        
           tmpdir, job_results_dir = calc_setup(atoms) 
        
           # Run phonopy 
        
           phonon.forces = forces 
        
           phonon.produce_force_constants() 
        
           phonon.run_mesh(with_eigenvectors=True) 
        
           phonon.run_total_dos() 
        
           phonon.run_thermal_properties(t_step=t_step, t_max=t_max, t_min=t_min) 
        
           phonon.auto_band_structure( 
        
               write_yaml=True, filename=Path(tmpdir, "phonopy_auto_band_structure.yaml") 
        
           ) 
        
           phonon.save(Path(tmpdir, "phonopy.yaml"), settings={"force_constants": True}) 
        
           phonon.directory = job_results_dir

If you are writing to disk after the calculation completes and a schema is returned, you can query schema["dir_name"] to get the path where the results folder is. In any case, there is going to need to be a fairly significant amount of refactoring here to ensure that your I/O goes where you expect it to go. These functions may have been working fine for you because you are using FireWorks which cd's into a custom directory for each run, but that is very unique to FireWorks. If you were to run these calculations without a workflow engine, for instance, you'll see that the files are not being written out to the unique results folder for that calculation.

Most of your issues in this PR boil down to the fact that you are trying to put too much into the recipes themselves, which are meant to be very well-defined units of work. In reality, most of your additions here should be quacc.runners functions that are called within your new recipes. I/O happens in runners and schemas depending on if it's meant to be done before/during or after the calculation. If using ChatGPT, it has no knowledge of this.

Andrew-S-Rosen · 2024-05-24T03:12:53Z

pyproject.toml

@@ -49,7 +49,7 @@ defects = ["pymatgen-analysis-defects>=2023.8.22", "shakenbreak>=3.2.0"]
 jobflow = ["jobflow[fireworks]>=0.1.14", "jobflow-remote>=0.1.0"]
 mlp = ["matgl>=1.0.0", "chgnet>=0.3.3", "mace-torch>=0.3.3", "torch-dftd>=0.4.0"]
 mp = ["atomate2>=0.0.14"]
-newtonnet = ["newtonnet>=1.1"]
+newtonnet = ["newtonnet>=1.1", "geodesic-interpolate @ git+https://github.com/virtualzx-nad/geodesic-interpolate.git"]


We can't have direct git links in pyproject.toml (unfortunately) because PyPI does not allow us to upload new version of quacc with URLs. The best course of action here is to simply make it clear in the @requires decorator that this package should be installed.

Hi @Andrew-S-Rosen ,
I want to have tests that run for geodesic interpolate on GitHub. Is there a way you see to do that in case we are not able to put URLs?
Thanks.

@kumaranu: Simply remove the direct link from pyproject.toml. The requirements.txt files in the tests folder are what is used by CI.

src/quacc/recipes/newtonnet/ts.py

kumaranu · 2024-06-10T20:45:27Z

Hi @Andrew-S-Rosen

I have added a function in runners/ase.py with its test.

My test is running alright locally but it is complaining that the required packages are not installed such as "sella", "NewtonNet", "geodesic" etc.
https://github.com/Quantum-Accelerators/quacc/actions/runs/9449377214/job/26025492513?pr=2176

Maybe the reason for this problem is that requirements-newtonnet.txt is not called for the packages that are present inside runners/ase.py. But I do not know which file will used to get that list of required packages.

Please help resolve this issue. Thanks.

Tagging Sam here as I was discussing this issue with him recently.
@samblau

kumaranu · 2024-06-10T23:19:40Z

Hi @Andrew-S-Rosen

I have added a function in runners/ase.py with its test.

My test is running alright locally but it is complaining that the required packages are not installed such as "sella", "NewtonNet", "geodesic" etc. https://github.com/Quantum-Accelerators/quacc/actions/runs/9449377214/job/26025492513?pr=2176

Maybe the reason for this problem is that requirements-newtonnet.txt is not called for the packages that are present inside runners/ase.py. But I do not know which file will used to get that list of required packages.

Please help resolve this issue. Thanks.

Tagging Sam here as I was discussing this issue with him recently. @samblau

I added the following three lines to requirements.txt inside the tests directory to solve that problem:
newtonnet==1.1.1
geodesic-interpolate @ git+https://github.com/virtualzx-nad/geodesic-interpolate.git
sella==2.3.4

Andrew-S-Rosen · 2024-06-10T23:22:26Z

tests/core/recipes/orca_recipes/test_orca_recipes.py

@@ -133,7 +133,7 @@ def test_ase_relax_job(tmp_path, monkeypatch):
        output["parameters"]["orcasimpleinput"]
        == "def2-tzvp engrad normalprint wb97x-d3bj xyzfile"
    )
-    assert output["parameters_opt"]["fmax"] == 0.1


Please don't change pre-existing tests. They should pass just fine.

Andrew-S-Rosen · 2024-06-10T23:22:37Z

pyproject.toml

@@ -50,7 +50,7 @@ jobflow = ["jobflow[fireworks]>=0.1.14", "jobflow-remote>=0.1.0"]
 mlp = ["matgl>=1.1.2", "chgnet>=0.3.3", "mace-torch>=0.3.3", "torch-dftd>=0.4.0"]
 mp = ["atomate2>=0.0.14"]
 newtonnet = ["newtonnet>=1.1"]
-parsl = ["parsl[monitoring]>=2024.5.27; platform_system!='Windows'"]


Please don't change unrelated versions. They should work just fine.

I don't know what was changed here. I see that this file is the same as the one in Quantum-Accelerator: main. Maybe you changed it back already.

Did I change the parsl version from 2023.10.23 to 2.24.5.27?

Yes, I took care of it. However, there are several other changes that still need to be reverted back, as shown in the file comparison view.

kumaranu · 2024-06-10T23:23:29Z

Hi @Andrew-S-Rosen
I have added a function in runners/ase.py with its test.
My test is running alright locally but it is complaining that the required packages are not installed such as "sella", "NewtonNet", "geodesic" etc. https://github.com/Quantum-Accelerators/quacc/actions/runs/9449377214/job/26025492513?pr=2176
Maybe the reason for this problem is that requirements-newtonnet.txt is not called for the packages that are present inside runners/ase.py. But I do not know which file will used to get that list of required packages.
Please help resolve this issue. Thanks.
Tagging Sam here as I was discussing this issue with him recently. @samblau

I added the following three lines to requirements.txt inside the tests directory to solve that problem: newtonnet==1.1.1 geodesic-interpolate @ git+https://github.com/virtualzx-nad/geodesic-interpolate.git sella==2.3.4

Although the original problem is solved, I saw two new errors, tblite and mlp tests both of which I did not touch.

mlp test were just complaining that they could not find mace. I added the following lines in the requirements.txt to get rid of that error.
mace-torch==0.3.4
torch-dftd==0.4.0

However, the error in tblite seems much different as the tests ran fine but the numbers don't match. I have left that as it is:
https://github.com/Quantum-Accelerators/quacc/actions/runs/9456768276/job/26049315963?pr=2176

Andrew-S-Rosen · 2024-06-10T23:31:29Z

@kumaranu: The tests/requirements.txt file cannot have the various packages you have added to it. It is meant to include a minimum set of packages corresponding to the tests-core CI run. If your tests require things like mace, they need to be running with the tests-mlps run. The reason the TBLite issues are happening is because TBLite is not compatible with torch (tblite/tblite#110).

I do not quite understand what your goal is here with tests/requirements.txt. You have added NewtonNet tests, which should only run if NewtonNet is installed.

kumaranu · 2024-06-11T17:22:48Z

@kumaranu: The tests/requirements.txt file cannot have the various packages you have added to it. It is meant to include a minimum set of packages corresponding to the tests-core CI run. If your tests require things like mace, they need to be running with the tests-mlps run. The reason the TBLite issues are happening is because TBLite is not compatible with torch (tblite/tblite#110).

I do not quite understand what your goal is here with tests/requirements.txt. You have added NewtonNet tests, which should only run if NewtonNet is installed.

I have removed NewtonNet and Sella from the test_ase.py requirements inside runners directory. It is using BFGS and EMT now.
@Andrew-S-Rosen @samblau

Andrew-S-Rosen · 2024-06-11T19:53:55Z

@kumaranu: Thanks. I will review the PR once you undo all the unrelated changes you've made (there are still many) and remove commented out code blocks.

src/quacc/recipes/newtonnet/ts.py

Andrew-S-Rosen · 2024-06-11T20:28:40Z

src/quacc/runners/ase.py

@@ -320,6 +326,153 @@ def run_vib(
    return vib


+def run_path_opt(


Let's rename this run_neb.

Andrew-S-Rosen · 2024-06-11T20:30:44Z

src/quacc/schemas/ase.py

@@ -275,6 +275,40 @@ def summarize_vib_and_thermo(
    )


+def summarize_path_opt_run(dyn: Optimizer) -> OptSchema:


And then we can rename this summarize_neb_run. It also needs a proper docstring.

src/quacc/schemas/ase.py

Andrew-S-Rosen · 2024-06-11T20:31:21Z

tests/core/recipes/vasp_recipes/mocked/test_vasp_recipes.py

Some more changes to revert later.

Andrew-S-Rosen · 2024-06-11T20:31:36Z

tests/core/schemas/test_ase.py

Some more changes to revert later.

src/quacc/runners/ase.py

Andrew-S-Rosen

Hi @kumaranu. I have provided an initial review below. There are many areas for cleanup, but overall the logic seems mostly reasonable. If you can address these changes, that would be ideal. Please note that you should also be checking GitHub Actions CI to see what tests fail. You'll see that your docs and pre-commit CI runners failed. Clicking the "details" will show you why.

Once you make these changes, I will review it once more and can assist with remaining issues. Thank you. Please also "resolve" the comments when you are done addressing them.

src/quacc/__init__.py

src/quacc/recipes/newtonnet/ts.py

tests/core/runners/test_ase.py

Andrew-S-Rosen · 2024-06-15T04:51:47Z

tests/core/runners/test_ase.py

+        )
+    ],
+)
+def test_run_neb(


Your test of run_neb should not rely on geodesic-interpolate. We want to make sure that we are only testing one thing at a time (hence the name a "unit test").

The NEB function needs list of ASE images. I am avoiding it because it will just become a long list of coordinates in this file. Not sure how to do it cleanly.

tests/core/runners/test_ase.py

Andrew-S-Rosen · 2024-06-15T04:52:57Z

tests/core/runners/test_ase.py

+    assert neb_summary["trajectory_results"][1]["energy"] == pytest.approx(
+        1.098, abs=0.01
+    )


A more thorough test of the output schema is required. Also, in general, let's not use such a loose tolerance if possible. This will help us capture errors appropriately.

Same problem as the geodesic here because I am using geodesic as the precursor to this step.

There are too many things being tested here at once is the major problem. For instance, runners/test_ase.py should be testing the runner: run_neb. It should be asserting that run_neb() functions appropriately and nothing else. There should be no major precursor to that step and nothing afterwards (i.e. no summarizing).

You'd then make a separate test for summarizing (e.g. tests.cores.schemas) and so on and so forth.

The idea of a "unit test" is that you are testing a specific unit of work.

Andrew-S-Rosen · 2024-06-15T04:53:09Z

tests/core/schemas/test_ase.py

Please do not make unrelated changes in your PR like the code changes in this file.

Removed move_magmoms from neb summary function.

fixed some docs.

corrected the test_run_neb2 functions.

…overwriting trajectory variable.

…ile.

kumaranu · 2024-07-29T21:33:12Z

Hi @Andrew-S-Rosen,

Thanks a lot for providing the comments above. I have finished going through them all.

As you mention two things where you might need to pitch in:

run_neb method in the Runner class
summarize_opt_run with sumarize_neb_run.

I have left those two unresolved.

Tagging @samblau here just for the update.

Andrew-S-Rosen · 2024-07-29T21:48:58Z

Thanks, @kumaranu! I recognize that this has likely taken longer than you anticipated and involved more iterations than desired. That said, I feel that the end result here is much more robust and will ensure that your work can continue to be well-maintained into the future.

I am starting my new position Aug. 1st so can't wrap this up immediately, but I will keep it on my to-do list for the near future (middle of August most likely). As you suggested, the two things that need to happen before the final merge are: 1) Ensuring that the run_neb function becomes a method of the Runner class rather than a standalone function; 2) Ensuring that the summarize_neb_run function become a method of the Summarize class rather than a standalone function. Both scenarios here are meant to minimize code duplication so that changes to one part of the code are properly reflected in your NEB runner and schema. I will ping you when that's done so you can look it over before we wrap this up.

…ptimization. This was the cause of the error.

kumaranu changed the title ~~Neb nn~~ WIP: Added ase Nudged Elastic Band in the NewtonNet recipe May 24, 2024

Andrew-S-Rosen reviewed May 24, 2024

View reviewed changes

Andrew-S-Rosen marked this pull request as draft May 31, 2024 03:15

Andrew-S-Rosen reviewed Jun 10, 2024

View reviewed changes